fix(thinking-block-sanitize): protect continuations by shape, not by tail distance - #279
Conversation
…tail distance planSanitize protected a tool-continuation's thinking only while it was the LATEST assistant turn (i === latestAsst). The predicate that matters — "is this turn's terminal tool_use answered by a following tool_result" — is a function of the message and what follows it, not of its distance from the tail. The two agree while the continuation is at the tail; they diverge the moment another turn lands after it, and the gate then flipped a byte-identical message from protected to stripped — a mid-history mutation the proxy itself causes, on every request where a continuation ages out of the tail, which is exactly the cache re-write this extension exists to prevent. Measured before the fix by an offline cross-request stability check over live captures: 133 violations over 563 requests on one session, 76 over 169 on another, all attributed to this extension. Zero after. A continuation deep in history keeps its thinking exactly as first sent: byte-stable, and the shape the API already accepted. Dropping it later buys nothing — the 400 this extension prevents concerns the latest turn — and costs a full prefix re-write. Regression test pins the v2StripSigned arm (v1 only drops omitted thinking, so the signed path is where the old gate bit): against the pre-fix planSanitize it fails, with the fix all 47 pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![vsits-codex-review-agent[bot]](https://avatars.githubusercontent.com/u/115404?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: PR #279 thinking-block-sanitize tail-distance removal
Date: 2026-07-31
Reviewed: PR #279 at 0f1920efb6d694cceebf03154c19612f0009c105
Round: 1
Label applied: changes-requested
What Is Correct
- The underlying cache-stability problem is real in the current planner: before this patch, the same answered tool-continuation can be forwarded byte-identically while it is the latest assistant turn and then be rewritten once a later turn lands. The new regression test in
test/proxy-thinking-block-sanitize.test.mjs:873demonstrates that instability clearly. isActiveToolContinuation()itself is safe to call at arbitrary depth. It only inspectsmessages[idx], requires the terminal block to be atool_usewith an id, and then scans later messages for a matchingtool_result.tool_use_id; I did not find any hidden tail-only indexing assumption inproxy/extensions/thinking-block-sanitize.mjs:106-115.- The full test suite passes at the PR head on this host:
1431/1431green vianode --test.
Blockers
-
proxy/extensions/thinking-block-sanitize.mjs:173now protects every answered tool-continuation, not just the latest one. That is safe for v1's omitted-thinking path, but it is not safe for v2's tools-hash-mismatch path, because v2's accepted contract is the opposite: on hash mismatch, strip signed thinking from all prior assistant turns, preserving only the latest active continuation. The v2 directive states that explicitly (docs/directives/proxy-thinking-block-sanitize-v2.md:58-67,:153-159), and the current tests already encode it intest/proxy-thinking-block-sanitize.test.mjs:343-388and:390-418.Concrete repro from the code under review:
- PR head
planSanitize([...answered continuation..., later assistant], { v2StripSigned: true })leaves the prior continuation's signed thinking intact. - Current
mainstrips that same prior signed-thinking block (droppedV2: 1).
That means this patch regresses the v2 mitigation by preserving historical signed thinking precisely in the mode that exists to remove structurally stale signatures after a tools-surface change. The review brief asked whether removing the latest-only gate is safe; for v2, it is not.
- PR head
What Needs Attention
- The new test only exercises
v2StripSigned: true, which is the right place to expose the current instability, but it does not distinguish the two mode contracts. The fix needs to preserve byte stability for v1 continuations without weakening v2's "strip all prior signed thinking on mismatch" rule. - The measurement comments added in
proxy/extensions/thinking-block-sanitize.mjs:142-159andtest/proxy-thinking-block-sanitize.test.mjs:860-872are not independently verifiable from this repo. I am not treating the133/563and76/169figures as established facts for review purposes.
Bloat / Non-Functional
None.
Recommendations
- Split the planner behavior by mode instead of deleting the tail gate globally. The repo's own history supports two different safety rules:
- v1: protecting an answered continuation by message shape is defensible, because v1 only strips omitted thinking and the documented 400 is latest-turn-scoped.
- v2: keep stripping signed thinking from historical continuations on tools-hash mismatch, preserving only the latest active continuation as the directive currently requires.
- Add an explicit regression test for the blocked case: a historical answered continuation under
v2StripSigned: truewith a later assistant turn present should still lose its signed thinking on mismatch.
Bottom Line
Revise before merge. The patch fixes a real byte-stability bug, but it does so by changing a shared guard that v2 depends on for safety. As written, PR #279 weakens the v2 mismatch sanitizer and can preserve historical signed thinking that the accepted v2 design requires us to strip. — Codex review
vsits-codex-review-agent
Bot
added
reviewed-by-codex-agent
|
Review result: changes requested. The bug you found is real and worth fixing — but the fix is too broad, and the part it over-reaches into is the part that protects against a hard failure rather than a cost regression. Confirmed: Blocker: removing the gate globally is safe for v1 but regresses v2. The v2 contract is the opposite of v1's — on a tools-hash mismatch, strip signed thinking from all prior assistant turns, preserving only the latest active continuation. That's stated in There's a further argument for keeping v2 as-is that strengthens the case. Suggested shape: split by mode rather than deleting the gate. Protect by shape under v1; keep the latest-only rule under v2. A regression test for the v2 case — historical answered continuation under On the measurements (133/563 and 76/169): those come from your out-of-tree tooling and we can't reproduce them here, so they're recorded as your findings rather than independently confirmed. That doesn't diminish them — the instability is visible in the code regardless. Context for why we're being careful: this extension runs in production on our host with Also worth noting: our own session telemetry is currently showing Happy to re-review once it's split by mode. — Proxy Builder |
1 participant
planSanitizeprotects a tool-continuation's thinking only while it is the latest assistant turn (i === latestAsst). But the property that makes it protected — "terminaltool_useanswered by a followingtool_result" — belongs to the message, not to its distance from the tail. The moment a later turn lands, the same byte-identical message flips from protected to stripped: a mid-history mutation the proxy itself causes on every request where a continuation ages out of the tail — i.e., exactly the whole-prefix cache re-write this extension exists to prevent.Measured (offline cross-request stability check over live captures, per-extension attribution): 133 violations over 563 requests on one session, 76 over 169 on another — every one attributed to this extension. Zero after the fix.
The fix: drop the
latestAssistantIndexgate; protect anyisActiveToolContinuation(messages, i). A continuation deep in history keeps its thinking exactly as first sent — byte-stable, and the shape the API already accepted. Stripping it later buys nothing (the 400 this extension prevents concerns the latest turn) and costs a full prefix re-write.Regression test pins the
v2StripSignedarm — the v1 path only drops omitted thinking, so signed thinking is where the old gate bit. Against unpatchedmainthe new test fails; with the fix, all 47 pass.Standalone — no dependency on the #272–#278 series (the measurement tooling that found it is #276).
🤖 Generated with Claude Code